Added hp and rp into bulk and ondelete modes #642
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds HTTP proxy and RPC proxy support to both bulk and on-delete update strategies, extending the update plan functionality that was previously only available for query trackers and schedulers. The changes refactor existing tests into a more maintainable table-driven approach using Ginkgo's DescribeTableSubtree and add pre-check validation logic for several components.
Changes:
- Extended bulk and on-delete update strategies to support HTTP proxies and RPC proxies in addition to existing components
- Refactored update strategy tests to use table-driven approach for better test coverage and maintainability
- Added UpdatePreCheck methods for Discovery, ControllerAgent, and MasterCache components to validate cluster state before updates
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/ytsaurus_controller_test.go | Refactored update plan strategy tests to use table-driven approach covering 7 components for bulk strategy and 4 for on-delete strategy; fixed context usage in WhoAmI call |
| test/e2e/helpers_test.go | Added helper functions for setting and getting component images dynamically based on component type |
| pkg/components/rpcproxy.go | Added support for both bulk and on-delete update strategies with appropriate strategy routing |
| pkg/components/httpproxy.go | Added support for both bulk and on-delete update strategies with appropriate strategy routing |
| pkg/components/master.go | Added support for both bulk and on-delete update strategies with appropriate strategy routing |
| pkg/components/master_caches.go | Added dependency injection of YtsaurusClient and implemented UpdatePreCheck for instance count validation |
| pkg/components/discovery.go | Added dependency injection of YtsaurusClient and implemented UpdatePreCheck for instance count and orchid data validation |
| pkg/components/controller_agent.go | Added dependency injection of YtsaurusClient and implemented UpdatePreCheck for connection state and maintenance checks |
| pkg/components/helpers.go | Refactored handleBulkUpdatingClusterState to use switch statement for better clarity; added nil check for Strategy in getComponentUpdateStrategy |
| pkg/components/ytsaurus_client.go | Extracted master quorum health check into separate method with maintenance flag support and improved error messages |
| controllers/component_manager.go | Updated component initialization to inject YtsaurusClient dependency into Discovery, ControllerAgent, and MasterCache |
| pkg/testutil/spec_builders.go | Fixed type error by using corev1.ServiceType instead of string literal for HTTPProxiesSpec |
| .github/workflows/subflow_run_e2e_tests.yaml | Split update strategy tests into separate job and increased max-parallel from 5 to 6 for better test parallelization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func setComponentImage(ytsaurus *ytv1.Ytsaurus, componentType consts.ComponentType, image string) { | ||
| switch componentType { | ||
| case consts.QueryTrackerType: | ||
| ytsaurus.Spec.QueryTrackers.Image = ptr.To(image) | ||
| case consts.MasterType: | ||
| ytsaurus.Spec.CoreImage = image | ||
| case consts.DiscoveryType: | ||
| ytsaurus.Spec.Discovery.Image = ptr.To(image) | ||
| case consts.MasterCacheType: | ||
| ytsaurus.Spec.MasterCaches.Image = ptr.To(image) | ||
| case consts.ControllerAgentType: | ||
| ytsaurus.Spec.ControllerAgents.Image = ptr.To(image) | ||
| case consts.RpcProxyType: | ||
| ytsaurus.Spec.RPCProxies[0].Image = ptr.To(image) | ||
| case consts.HttpProxyType: | ||
| ytsaurus.Spec.HTTPProxies[0].Image = ptr.To(image) | ||
| case consts.SchedulerType: | ||
| ytsaurus.Spec.Schedulers.Image = ptr.To(image) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function setComponentImage may cause nil pointer dereferences when setting images. For example, if ytsaurus.Spec.QueryTrackers, ytsaurus.Spec.MasterCaches, ytsaurus.Spec.Schedulers, ytsaurus.Spec.RPCProxies[0], or ytsaurus.Spec.HTTPProxies[0] are nil, the assignment will panic. Consider adding nil checks before setting image values to prevent runtime panics.
| case consts.HttpProxyType: | ||
| ytsaurus.Spec.HTTPProxies[0].Image = ptr.To(image) | ||
| case consts.SchedulerType: | ||
| ytsaurus.Spec.Schedulers.Image = ptr.To(image) |
There was a problem hiding this comment.
The switch statement is missing a default case. If an unsupported component type is passed, the function will silently do nothing, which could lead to bugs that are hard to debug. Consider adding a default case that panics or logs an error to make it clear when an unsupported component type is used.
| ytsaurus.Spec.Schedulers.Image = ptr.To(image) | |
| ytsaurus.Spec.Schedulers.Image = ptr.To(image) | |
| default: | |
| Fail(fmt.Sprintf("unsupported component type in setComponentImage: %v", componentType)) |
| func getComponentImage(ytsaurus *ytv1.Ytsaurus, componentType consts.ComponentType) string { | ||
| switch componentType { | ||
| case consts.QueryTrackerType: | ||
| return *ytsaurus.Spec.QueryTrackers.Image | ||
| case consts.MasterType: | ||
| return ytsaurus.Spec.CoreImage | ||
| case consts.DiscoveryType: | ||
| return *ytsaurus.Spec.Discovery.Image | ||
| case consts.MasterCacheType: | ||
| return *ytsaurus.Spec.MasterCaches.Image | ||
| case consts.ControllerAgentType: | ||
| return *ytsaurus.Spec.ControllerAgents.Image | ||
| case consts.RpcProxyType: | ||
| return *ytsaurus.Spec.RPCProxies[0].Image | ||
| case consts.HttpProxyType: | ||
| return *ytsaurus.Spec.HTTPProxies[0].Image | ||
| case consts.SchedulerType: | ||
| return *ytsaurus.Spec.Schedulers.Image | ||
| default: | ||
| return "unsupported component type" | ||
| } | ||
| } |
There was a problem hiding this comment.
The function getComponentImage may return empty strings for pointer fields when they are nil, leading to potential nil pointer dereferences. For example, if ytsaurus.Spec.QueryTrackers is nil or ytsaurus.Spec.QueryTrackers.Image is nil, dereferencing the pointer will cause a panic. Consider adding nil checks before dereferencing pointers.
2 participants
No description provided.